Skip to content

[Feature] Spherical Harmonics Support (Viser 0.2.23)#399

Open
Crezle wants to merge 21 commits intonerfstudio-project:mainfrom
Crezle:extended_gsplat_properties
Open

[Feature] Spherical Harmonics Support (Viser 0.2.23)#399
Crezle wants to merge 21 commits intonerfstudio-project:mainfrom
Crezle:extended_gsplat_properties

Conversation

@Crezle
Copy link

@Crezle Crezle commented Feb 18, 2025

Inspired by PR #286, this feature aims to add further functionality for "add_gaussian_splats()" where view-dependent effects can be represented. This is often useful due to exported splat models often including Spherical Harmonics variables in addition to base color.

This PR also contains some traces of attempted surface normal implementation, but without any .PLY files including non-zero normals, this will be covered in a separate PR.

@Crezle Crezle marked this pull request as ready for review February 18, 2025 13:12
@Crezle
Copy link
Author

Crezle commented Feb 18, 2025

This should be backward compatible with former use of "gaussian_splats.py" (e.g. if SH-coeffs are not provided).
Only concern is that performance might be a tiny bit reduced due to larger packets having to be sent due to now 45 additional coefficients being sent (each at 16 bits).

@brentyi
Copy link
Collaborator

brentyi commented Mar 23, 2025

Hi @Crezle, sorry for the late response here!

This looks great to me, would love to merge support for SH in. 🙂

I know @AntonioMacaronio and @beckyfeng08 also spent a fair amount of time on SH support in the shader; I'm interested in their thoughts.

For getting this merged:

  • The main thing I can think of is to eliminate the performance penalty when SH coeffs aren't specified. An extra 45 coefficients per Gaussian is a lot. In the shader itself, we could also do some preprocessor conditionals (#ifdef etc) to prevent the SH code from running when there are no coefficients.
  • From a glance it looks like the rgbs arg is unused when sh_coeffs is not None, if so this feels a bit weird?

@Crezle
Copy link
Author

Crezle commented Mar 24, 2025

Hi @Crezle, sorry for the late response here!

This looks great to me, would love to merge support for SH in. 🙂

I know @AntonioMacaronio and @beckyfeng08 also spent a fair amount of time on SH support in the shader; I'm interested in their thoughts.

For getting this merged:

* The main thing I can think of is to eliminate the performance penalty when SH coeffs aren't specified. An extra 45 coefficients per Gaussian is a lot. In the shader itself, we could also do some preprocessor conditionals (`#ifdef` etc) to prevent the SH code from running when there are no coefficients.

* From a glance it looks like the `rgbs` arg is unused when `sh_coeffs is not None`, if so this feels a bit weird?

Thank you for the response @brentyi! As I see it is that general 3DGS exports always contain 48 coefficients, where the first 3 are RGB. If we consider that we always have all the coefficients supplied, we can just ignore the RGB argument, but I assume we still want to keep the flexibility of using constant colors.

So, somehow we want to require at least 3 out of the 48 coefficients.
I guess we can set a size requirement for the input SH coefficents, where we expect it to either have 3 or 48 coefficients.

To also respect the desire to not send a bunch of zeros if we only have 3 coefficients, we could try to handle this at client-end, where it can dynamically handle a message consisting of either 3 or 48 coefficients.

What do you think?

@brentyi
Copy link
Collaborator

brentyi commented Mar 28, 2025

In that case: to simplify the API, maybe we just add a separate Python function for adding the splats with SH support? Maybe like add_gaussian_splats_sh().

To also respect the desire to not send a bunch of zeros if we only have 3 coefficients, we could try to handle this at client-end, where it can dynamically handle a message consisting of either 3 or 48 coefficients.

Yeah, this sounds good. I think the networking overhead and shader overhead here are ~equally important. Since most applications are fine without SH it would be great if there was no additional overhead associated with it!

// multiplications.
// A comprehensible table of Real SH constants:
// https://en.wikipedia.org/wiki/Table_of_spherical_harmonics
vec3 viewDir = normalize(center - cameraPosition);
Copy link

@AntonioMacaronio AntonioMacaronio Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only comment I have (I think @beckyfeng08 was the one who told me) is that the viewing direction is actually a bug here and it should depend on T_camera_group for a proper calculation

In my PR, I have swapped this calculation to this:

vec3 t_group_camera = -(transpose(mat3(T_camera_group)) * T_camera_group[3].xyz);
vec3 viewDir = normalize(center - t_group_camera);

Here is a link to this code in my PR

also oops i did not mean to make this a review i was just trying to comment asdfghjk - really nice work though @Crezle !!!!

@AntonioMacaronio
Copy link

AntonioMacaronio commented Apr 16, 2025

Not an issue with this PR but I'm getting a weird error where I can't load a splat anymore on a dev version of viser (viser installed with pip install -e .)
image

However, whenever viser is installed normally with pip install viser this issue does not arise 👀

@Crezle
Copy link
Author

Crezle commented Apr 17, 2025

Yeah, I discovered something similar myself amongst with other fellow students. Seems to be an issue since I merged changes from main into this branch.

Seems like it was fixed with the last issue, but me and the others will take a look at it soon👍

@AntonioMacaronio
Copy link

Can confirm it's fixed!! thank you brent

@Crezle
Copy link
Author

Crezle commented May 24, 2025

Hi, I just wanted to update that I am going to look at this later on and have not "abandoned" this PR.
Current focus is just to complete my thesis 👍

@brentyi
Copy link
Collaborator

brentyi commented May 27, 2025

Best of luck @Crezle, and thanks for the update!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants